Remove some GT_ASG_op leftovers #18205
Conversation
mikedn
force-pushed
the
asg-cleanup
branch
3 times, most recently
from
b13146f
to
bf7a400
Compare
|
Is it ready for review? |
|
Hmm, looks like it was. I only now notice that it has conflicts. |
|
|
||
| assert(lpIterTree); |
There was a problem hiding this comment.
I find it odd to assert that a pointer is non-null when you're going to dereference it on the next line anyway. Such asserts are typically used when you store the pointer for later use and want to avoid wasting your time trying to figure out how did the null get there.
src/jit/rangecheck.cpp
Outdated
| @@ -875,29 +859,15 @@ Range RangeCheck::ComputeRangeForLocalDef(BasicBlock* block, | |||
| JITDUMP("----------------------------------------------------\n"); | |||
| } | |||
| #endif | |||
| switch (asg->OperGet()) | |||
| Range range = GetRange(asgBlock, asg->gtGetOp2(), monotonic DEBUGARG(indent)); | |||
There was a problem hiding this comment.
Could you please add an assert that asg->OperIs(GT_ASG) just to make my paranoia calm?
There was a problem hiding this comment.
Seems a bit redundant but I'll add it. Hopefully GT_ASG will disappear one day and take the assert with it :)
|
cc @dotnet/jit-contrib |
|
Assert added, conflicts fixed. Turns out that in one case I managed to update the same comment in another PR and used different wording. Erm. |
|
#19613 (comment) |
|
@dotnet-bot test this please |
|
close/reopen has helped. |
src/jit/compiler.h
Outdated
| @@ -4098,8 +4098,8 @@ class Compiler | |||
|
|
|||
| void fgInterBlockLocalVarLiveness(); | |||
|
|
|||
| // The presence of "x op= y" operations presents some difficulties for SSA: this is both a use of some SSA name of | |||
| // "x", and a def of a new SSA name for "x". The tree only has one local variable for "x", so it has to choose | |||
| // The presence of struct field assignment presents some difficulties for SSA: this is both a use of some SSA name | |||
There was a problem hiding this comment.
I think there are cases of partial assignment where even a primitive type lclVar can have a partial assignment (obviously not safe or verifiable code, of course). Perhaps this would be better as "The presence of partial assignments, e.g. a struct field assignment or a GT_LCL_FLD assignment of a portion of a local, presents ..."
There was a problem hiding this comment.
Right, *((byte*)&i) = … does produce a partial def. It's a byte LCL_FLD so it blocks enregistration but it's not address exposed so it does appear in SSA.
I've update this and another SsaBuilder comment to only say "partial definition" (seems to used more commonly than "partial assignment") and extended the GTF_VAR_USEASG comment to say what a partial definition is:
#define GTF_VAR_USEASG 0x40000000 // GT_LCL_VAR -- this is a partial definition, a use of the previous definition is implied
// A partial definition usually occurs when a struct field is assigned to (s.f = ...) or
// when a scalar typed variable is assigned to via a narrow store (*((byte*)&i) = ...).
No description provided.